Skip to content

Conversation

@rimbi
Copy link

@rimbi rimbi commented Sep 15, 2023

This is the PR upgrade dependencies to polkadot v1.1.0.

xlc and others added 8 commits August 8, 2023 13:33
* vesting: can be used for gov

* wording
* emit LockSet on extend_lock

Signed-off-by: Gregory Hill <[email protected]>

* emit LockSet for existing lock, add tests

Signed-off-by: Gregory Hill <[email protected]>

---------

Signed-off-by: Gregory Hill <[email protected]>
* orml-parameters

* fix

* add tests

* whitespaces

* more tests

* 0.9.43

* add weights

* fix

* fix

* fix docs test

* fix

* Apply suggestions from code review

Co-authored-by: Nuno Alexandre <[email protected]>

* update

* improve usage

* fix

* update comment

* set weight

---------

Co-authored-by: Nuno Alexandre <[email protected]>
@rimbi rimbi requested a review from Agusrodri September 15, 2023 14:33
@rimbi rimbi force-pushed the cem-upgrade-to-polkadot-v1.1.0 branch from ce621b4 to a7a0861 Compare September 15, 2023 14:42
Copy link

@Agusrodri Agusrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a bit of inconsistency related to Cargo.toml files, dependencies and branches. In some places v1.0.0 is used and in some others v1.1.0. We should use v1.1.0 in all packages, and dependencies and commit branches should be consistent.

.gitignore Outdated
.DS_Store
.idea

.vscode

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid changing this kind of files. It is recommended to strictly change the necessary files associated to the update (Cargo.toml ones and those that require upstream changes).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👍

Cargo.dev.toml Outdated
sp-tracing = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" }
sp-wasm-interface = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" }
sp-io = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" }
sp-keystore = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that a lot of dependencies in this file are duplicated. You should only import them once. Also, I think the patches to the different repos (paritytech/cumulus, paritytech/polkadot, paritytech/substrate) are not needed anymore since they were all merged into the mono-repo. In that case, you should have one patch for the entire repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pr is not finalized yet.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@rimbi
Copy link
Author

rimbi commented Sep 15, 2023

I see a bit of inconsistency related to Cargo.toml files, dependencies and branches. In some places v1.0.0 is used and in some others v1.1.0. We should use v1.1.0 in all packages, and dependencies and commit branches should be consistent.

The PR is just a draft, not finalized. I have pushed so that you can see the failing tests. Also, only the patch section has been modified to point to new polkadot-sdk repo, which is enough for me to proceed right now.

@moonbeam-foundation moonbeam-foundation deleted a comment from rimbi Sep 15, 2023
@Agusrodri
Copy link

Agusrodri commented Sep 15, 2023

That's okay, I was pointing that there are duplicated dependencies because that might be the reason why some tests are failing, but will take a look at them now.

@Agusrodri
Copy link

Agusrodri commented Sep 15, 2023

Also @rimbi, can you please indicate which tests are failing? Thanks :)

Edit: I already saw that are the ones related to xtokens.

@Agusrodri
Copy link

Agusrodri commented Sep 16, 2023

After diving into the code, and as I saw while debugging the tests, it seems that there are not XCM upstream related changes that may affect the behavior of the xtokens tests.

I think the problem could be associated to the fact that there is one commit from upstream master branch that was never cherry-picked to our moonbeam-polkadot-v1.1.0 branch (f7c8b38) (also check 22ae492 and 7582d37 as they could be related as well).

As these tests are all passing on upstream master branch and there don't seem to be XCM related changes that may have changed the behavior, I would say almost for sure that the failures come due to a misconfiguration regarding this inconsistency between branches.

What you can do as a next step is to cherry-pick the missing commits into our moonbeam-polkadot-v1.1.0 branch, merge that branch into cem-upgrade-to-polkadot-v1.1.0, apply the necessary changes and start testing from that point. Also this will probably make you save a lot of work, as it won't be necessary to apply changes that were already applied in those commits.

Hope this helps! Let me know when you make more progress on it, I'll be glad to give a second look and help with tests :)

Edit for visibility: Indeed there is a change included upstream inside AllowTopLevelPaidExecutionFrom barrier, introduced in this commit.

@rimbi
Copy link
Author

rimbi commented Sep 18, 2023

After diving into the code, and as I saw while debugging the tests, it seems that there are not XCM upstream related changes that may affect the behavior of the xtokens tests.

I think the problem could be associated to the fact that there is one commit from upstream master branch that was never cherry-picked to our moonbeam-polkadot-v1.1.0 branch (f7c8b38) (also check 22ae492 and 7582d37 as they could be related as well).

As these tests are all passing on upstream master branch and there don't seem to be XCM related changes that may have changed the behavior, I would say almost for sure that the failures come due to a misconfiguration regarding this inconsistency between branches.

What you can do as a next step is to cherry-pick the missing commits into our moonbeam-polkadot-v1.1.0 branch, merge that branch into cem-upgrade-to-polkadot-v1.1.0, apply the necessary changes and start testing from that point. Also this will probably make you save a lot of work, as it won't be necessary to apply changes that were already applied in those commits.

Hope this helps! Let me know when you make more progress on it, I'll be glad to give a second look and help with tests :)

Thank you very much for the effort, Agustin 🙏 Checking it...

Looks like the changes in the polkadot-sdk causes the problem. I am still on it.

* update gh action concurrency

* update
@rimbi rimbi force-pushed the cem-upgrade-to-polkadot-v1.1.0 branch 3 times, most recently from a8425cc to 9998244 Compare September 19, 2023 11:50
@rimbi rimbi self-assigned this Sep 19, 2023
Copy link

@Agusrodri Agusrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me so far. We should pay attention to how upstream ORML will manage the case of the new restriction in the barrier (you can also open a PR upstream proposing to upgrade master with your changes).

@rimbi rimbi force-pushed the cem-upgrade-to-polkadot-v1.1.0 branch from 9998244 to f945515 Compare September 21, 2023 07:15
@rimbi
Copy link
Author

rimbi commented Sep 21, 2023

you can also open a PR upstream

Makes sense! Here is the PR. 👍

@rimbi rimbi force-pushed the cem-upgrade-to-polkadot-v1.1.0 branch from 55b4b6b to fdced49 Compare September 21, 2023 07:34
@rimbi rimbi marked this pull request as ready for review September 21, 2023 07:40
@rimbi
Copy link
Author

rimbi commented Sep 22, 2023

I am closing this PR and will open another one. The reason is that I have some updates in the base branch (which does not effect the current change set) but github does not take that into account and not recalculate the diffs.

@rimbi rimbi closed this Sep 22, 2023
@rimbi rimbi deleted the cem-upgrade-to-polkadot-v1.1.0 branch October 9, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants